Skip to content

Load grids with xarray and then create uxarray grid object#162

Merged
rajeeja merged 17 commits into
mainfrom
rajeeja/remove_open_dataset
Oct 26, 2022
Merged

Load grids with xarray and then create uxarray grid object#162
rajeeja merged 17 commits into
mainfrom
rajeeja/remove_open_dataset

Conversation

@rajeeja

@rajeeja rajeeja commented Oct 20, 2022

Copy link
Copy Markdown
Contributor

#151 and Discussion: #154

@philipc2 philipc2 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @rajeeja! I've left a couple comments about some slightly inconsistencies with casting face nodes to integers. Other than that, the implementation is looking great!

Comment thread uxarray/grid.py Outdated
Comment thread test/test_helpers.py Outdated
Comment thread uxarray/helpers.py Outdated
Comment thread uxarray/helpers.py Outdated
@philipc2

Copy link
Copy Markdown
Member

Small suggestion for the Grid constructor, but also applies to most of our other functions: Do we want to implement type hints?

Our current constructor for grid is:
__init__(self, dataset, **kwargs):

A simple change to emphasize that we are using xr.Dataset would be as follows
__init__(self, dataset : xr.Dataset, **kwargs):

@rajeeja

rajeeja commented Oct 24, 2022

Copy link
Copy Markdown
Contributor Author

Great work @rajeeja! I've left a couple comments about some slightly inconsistencies with casting face nodes to integers. Other than that, the implementation is looking great!

Thanks! I'll fix these later today.

@erogluorhan erogluorhan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank very much for doing this! Please address a few things as follows:

  • Reflect the removal of the dataset functionality in the documentation (e.g. user API, draft UXarray API, etc.)
  • Inline comments

Comment thread uxarray/grid.py Outdated
Comment thread uxarray/grid.py
Comment thread uxarray/grid.py Outdated
Comment thread uxarray/helpers.py Outdated
Comment thread uxarray/grid.py Outdated
Comment thread uxarray/grid.py
Comment thread uxarray/grid.py Outdated
Comment thread uxarray/_exodus.py

@erogluorhan erogluorhan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks very much for doing this!

@philipc2 philipc2 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you once again for putting this together @rajeeja. A few more comments on casting to integers, but nothing directly related to functionality. Great work!

Comment thread uxarray/helpers.py Outdated
Comment thread uxarray/_scrip.py Outdated
Comment thread uxarray/grid.py Outdated
Comment thread uxarray/_exodus.py Outdated
rajeeja and others added 2 commits October 26, 2022 09:11
Fixes #142

Co-authored-by: Orhan Eroglu <erogluorhan@users.noreply.github.com>

@philipc2 philipc2 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good! Great work

@rajeeja rajeeja merged commit 5075389 into main Oct 26, 2022
@erogluorhan erogluorhan deleted the rajeeja/remove_open_dataset branch November 2, 2022 03:01
@erogluorhan erogluorhan linked an issue Nov 15, 2022 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Separate dataset and grid object

3 participants